-
Notifications
You must be signed in to change notification settings - Fork 731
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement handling of non-actpass setup in offers #1090
Implement handling of non-actpass setup in offers #1090
Conversation
b57992b
to
0b82548
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1090 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 30 30
Lines 5868 5868
=========================================
Hits 5868 5868 ☔ View full report in Codecov by Sentry. |
4c4fa9a
to
1609289
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks correct to me.
if description.type == "offer" and media.dtls.role == "client": | ||
dtlsTransport._set_role(role="server") | ||
if description.type == "answer": | ||
dtlsTransport._set_role( | ||
role="server" if media.dtls.role == "client" else "client" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw what role do we end up with when none of these conditionals match?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auto
which I hope means "determined by who wins ICE"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand why we are only handling media.dtls.role == "client"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the handling of offers so the existing code assumes actpass. Lets see if I can prove that by adding more tests because who does not like more tests ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I suspected... the current code assumes role=auto in SRD. Which is not quite correct since auto means "determined by DTLS" whereas when we parse actpass we should be setting this to "client" explicitly. This is currently done in createAnswer which is a bit confusing but ok.
@@ -1272,9 +1274,6 @@ def __validate_description( | |||
if not media.ice.usernameFragment or not media.ice.password: | |||
raise ValueError("ICE username fragment or password is missing") | |||
|
|||
# check DTLS role is allowed | |||
if description.type == "offer" and media.dtls.role != "auto": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to also remove raising error in __validate_description
when peer's role in peer's offer is not actpass
?
aiortc/src/aiortc/rtcpeerconnection.py
Line 1277 in e9c13ea
raise ValueError("DTLS setup attribute must be 'actpass' for an offer") |
Hi @fippo ! Thanks for looking into this. I must admit I'm confused by the wording in RFC 8842, it stills say that offers should use https://datatracker.ietf.org/doc/html/rfc8842#name-generating-the-initial-sdp- |
Backward compatible in the sense "if you don't know the other side supports this keep using actpass but if you know it does (or assume)... then go ahead". For this to work we need more implementation (like aiortc) to support the new style ;-) |
Rebasing on top of |
1609289
to
15c3610
Compare
That fixed one job but another started failing 😂 |
RFC 8842 updated RFC 5763 to allow a differt setup attribute than actpass in offers which allows for more control of the DTLS role. See https://issues.webrtc.org/issues/42223106 for additional details Fixes aiortc#1087
15c3610
to
0b40293
Compare
Thanks so much @fippo! |
RFC 8842 updated RFC 5763 to allow a differt setup attribute than
actpass
in offers which allows for more control of the DTLS role.
See https://issues.webrtc.org/issues/42223106 for additional details
Fixes #1087